Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add datepicker to filters with datetime & date input fields (Meta #124) #61

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

marcwieland95
Copy link
Contributor

No description provided.

@marcwieland95 marcwieland95 self-assigned this Jul 18, 2024
Copy link

@renestalder renestalder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcwieland95 I think you should ask someone additional for a review on the PHP part here. I'm not a PHP developer and can barely validate the code health here. The only part I can tell for sure is that the HTML probably should be moved to Twig.


return sprintf(
'<input type="datetime-local" name="{name}" value="%s">',
$operator !== static::CRITERIA_IS_EMPTY ? $value : ''
'<input type="datetime-local" name="{name}" value="%s" data-controller="araise--core-bundle--datetime" data-araise--core-bundle--datetime-lang-value="%s">',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be moved to Twig?

public function getValueField(?string $value = null, ?string $operator = null): string
{
$date = \DateTime::createFromFormat(static::getQueryDataFormat(), (string) $value) ?: new \DateTime();
$value = $date->format(static::getDateFormat());

return sprintf(
'<input type="date" name="{name}" value="%s">',
$operator !== static::CRITERIA_IS_EMPTY ? $value : ''
'<input type="date" name="{name}" value="%s" data-controller="araise--core-bundle--datetime" data-araise--core-bundle--datetime-lang-value="%s">',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be moved to Twig?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@renestalder Technically yes, but Symfony itself on other types also isn't doing that for input or select fields.
I see the issue with writting Stimulus controllers/values which aren't readable. But that's how messy Stimulus is without a helper function. Since Symfony has this helper functions for TWIG, we should also be able to access it here in PHP. I'll have a look into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had no time to look into that. Made a note for later for myself

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcwieland95 you could use the StimulusHelper

@renestalder
Copy link

Assigned @tuxes3 for a review, as this needs a PHP developer to look at the changes.

@marcwieland95 marcwieland95 removed their assignment Jul 23, 2024
I had to handle the requestStack retrieval differently between those two classes. Don't know why this is the case.
@tuxes3 tuxes3 force-pushed the feature/124-filter-datepicker branch from e702cb0 to 72b3966 Compare July 23, 2024 17:40
Copy link
Contributor

@tuxes3 tuxes3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcwieland95 I rebased the branch, applied our codestyle and tested the implementation in our demo app. LGTM

@tuxes3 tuxes3 merged commit 3cc6982 into develop Jul 24, 2024
7 checks passed
@tuxes3 tuxes3 deleted the feature/124-filter-datepicker branch July 24, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants